"WLED Matrix tool": new image & scrolling text interface #4982
Conversation
WalkthroughAdds a Matrix Tool web UI and GIF codec, updates the build script to emit Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas to pay extra attention:
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wled00/data/style.css (1)
39-51: Indent with tabs per project guideline.New declarations must use tabs for indentation in
wled00/dataassets; the added lines currently use spaces.Apply this diff:
- transition: all 0.3s ease; + transition: all 0.3s ease; } -button:hover, .btn:hover{ - background:#555; - border-color:#555; -} +button:hover, .btn:hover{ + background:#555; + border-color:#555; +}As per coding guidelines
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
tools/cdata.js(3 hunks)wled00/data/index.htm(1 hunks)wled00/data/matrixtool/matrixtool.htm(1 hunks)wled00/data/matrixtool/omggif.js(1 hunks)wled00/data/pxmagic/pxmagic.htm(1 hunks)wled00/data/settings.htm(1 hunks)wled00/data/style.css(2 hunks)wled00/wled_server.cpp(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
wled00/data/**/*.{htm,html,css,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use tabs for indentation in web files (.htm/.html/.css/.js) under wled00/data
Files:
wled00/data/style.csswled00/data/matrixtool/omggif.jswled00/data/matrixtool/matrixtool.htmwled00/data/settings.htmwled00/data/pxmagic/pxmagic.htmwled00/data/index.htm
wled00/**/*.{cpp,h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for all C++ source and header files
Files:
wled00/wled_server.cpp
🧠 Learnings (2)
📚 Learning: 2025-09-20T09:00:08.942Z
Learnt from: CR
PR: wled/WLED#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-20T09:00:08.942Z
Learning: Applies to wled00/html_*.h : DO NOT edit generated headers wled00/html_*.h
Applied to files:
wled00/wled_server.cpptools/cdata.js
📚 Learning: 2025-09-14T18:43:59.338Z
Learnt from: blazoncek
PR: wled/WLED#4932
File: wled00/data/cpal/cpal.htm:14-14
Timestamp: 2025-09-14T18:43:59.338Z
Learning: In WLED, static web pages like cpal.htm are served directly at the root URL level (e.g., "/cpal.htm") via handleStaticContent() from embedded page data, regardless of their physical source folder location in the repository. Relative script references like "common.js" resolve correctly from the served URL path, not the source file path.
Applied to files:
tools/cdata.js
🪛 Biome (2.1.2)
wled00/data/matrixtool/omggif.js
[error] 438-438: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 464-464: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 465-465: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 466-466: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 467-467: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 468-468: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 469-469: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 470-470: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 471-471: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 472-472: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 473-473: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 474-474: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 475-475: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 483-483: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 512-512: This code is unreachable
(lint/correctness/noUnreachable)
[error] 32-32: Shouldn't redeclare 'gopts'. Consider to delete it or rename it.
'gopts' is defined here:
(lint/suspicious/noRedeclare)
[error] 32-32: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 32-32: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 448-448: Shouldn't redeclare 'block_size'. Consider to delete it or rename it.
'block_size' is defined here:
(lint/suspicious/noRedeclare)
[error] 477-477: Shouldn't redeclare 'has_local_palette'. Consider to delete it or rename it.
'has_local_palette' is defined here:
(lint/suspicious/noRedeclare)
[error] 487-487: Shouldn't redeclare 'block_size'. Consider to delete it or rename it.
'block_size' is defined here:
(lint/suspicious/noRedeclare)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
wled00/data/matrixtool/matrixtool.htm (7)
254-440: Consider accessibility enhancements.The UI structure is well-organized. For better accessibility, consider adding ARIA labels to interactive elements and ensuring full keyboard navigation support for the cropping canvas and context menus.
467-491: Segment filtering correctly prevents NaN dimensions.The implementation at lines 477-481 correctly filters out 1D segments (where
h > 1 && w > 1is false), which resolves the NaN dimension issue from previous discussions. The fallback values at line 489 (||16) provide safe defaults when segment data is missing.Consider showing errors to users.
Line 490 catches errors with
console.error, which logs to the console but doesn't notify the user. Consider using thetoast()function to display fetch errors:- }).catch(console.error); + }).catch(e => { console.error(e); toast('Failed to load segments', 'error'); });
537-563: Consider parallelizing image loading.The sequential image loading at lines 540-562 uses
awaitinside afor...ofloop, which loads images one at a time. For better performance, consider loading images in parallel:- for(const f of imgs){ + await Promise.all(imgs.map(async (f) => { const name=f.name.replace('/',''),url=`${wURL}/${name}`; // ... rest of loading logic - } + }));This will significantly improve load times when multiple images are present on the device.
786-799: Prefertoast()overalert()for consistency.Line 788 uses
alert()for the unsupported format message, which is inconsistent with thetoast()pattern used elsewhere in the codebase. Consider refactoring:function showUnsupported(url,name){ - const msg=`Image format not supported by WLED.\nPlease convert to GIF below or use the Pixel Magic Tool.`; - alert(msg); + toast('Image format not supported by WLED. Please convert to GIF below or use the Pixel Magic Tool.', 'error'); fetch(url)This improves UI consistency and user experience.
959-976: Simplify shadow disabling.Line 974 sets
shadowColorto"#000F"(transparent) to disable the shadow effect. A clearer approach is to setshadowBlurto 0:ctx.roundRect(crop.x, crop.y, crop.w, crop.h, 6); ctx.stroke(); - ctx.shadowColor = "#000F"; // disable shadow again by simply making it transparent + ctx.shadowBlur = 0; + ctx.shadowColor = "transparent"; updPrev(); }
1139-1156: Consider usingaddEventListenerfor context menu.Lines 1146-1148 use inline
onclickattributes in the context menu HTML, which can cause CSP violations and reduces maintainability. Consider refactoring to event listeners:menu.innerHTML=` - <button onclick="downloadImg()">Download</button> - <button class="danger" onclick="deleteImg()">Delete</button> - <button onclick="closeMenu()">Cancel</button>`; + <button data-action="download">Download</button> + <button class="danger" data-action="delete">Delete</button> + <button data-action="cancel">Cancel</button>`; + menu.querySelector('[data-action="download"]').onclick = downloadImg; + menu.querySelector('[data-action="delete"]').onclick = deleteImg; + menu.querySelector('[data-action="cancel"]').onclick = closeMenu;
1249-1255: Consider using named constants for effect IDs.The code uses magic numbers for effect IDs (e.g., 53 for image, 122 for scrolling text) throughout the file. Consider defining named constants at the top of the script for better maintainability:
const FX_IMAGE = 53; const FX_SCROLLING_TEXT = 122; // Then use throughout: function upTxt(){ const id=+gId('segTxt').value,txt=gId('txtIn').value.trim().slice(0,64); const j={on:true,seg:{id,fx:FX_SCROLLING_TEXT,n:txt,...}};This makes the code more self-documenting and easier to maintain if effect IDs change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/data/matrixtool/matrixtool.htm(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
wled00/data/**/*.{htm,html,css,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use tabs for indentation in web files (.htm/.html/.css/.js) under wled00/data
Files:
wled00/data/matrixtool/matrixtool.htm
wled00/data/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
wled00/data/**: When modifying web UI files, runnpm run buildto regenerate embedded headers before any firmware build
For web UI changes, edit files only under wled00/data (not firmware or generated files)
Files:
wled00/data/matrixtool/matrixtool.htm
🧠 Learnings (2)
📚 Learning: 2025-10-05T15:41:15.790Z
Learnt from: DedeHai
PR: wled/WLED#4982
File: wled00/data/matrixtool/omggif.js:32-35
Timestamp: 2025-10-05T15:41:15.790Z
Learning: The file `wled00/data/matrixtool/omggif.js` is an external third-party library from https://github.com/deanm/omggif by Dean McNamee and should not be reviewed for code issues as it's maintained externally.
Applied to files:
wled00/data/matrixtool/matrixtool.htm
📚 Learning: 2025-10-05T15:24:05.535Z
Learnt from: CR
PR: wled/WLED#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.535Z
Learning: Applies to wled00/data/**/*.{htm,html,css,js} : Use tabs for indentation in web files (.htm/.html/.css/.js) under wled00/data
Applied to files:
wled00/data/matrixtool/matrixtool.htm
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
🔇 Additional comments (1)
wled00/data/matrixtool/matrixtool.htm (1)
1-253: LGTM!The HTML structure and CSS styling are well-organized. The tab indentation follows the project guidelines for web assets under
wled00/data. The styling provides a comprehensive and polished UI for the Matrix Tool.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
wled00/data/matrixtool/matrixtool.htm (3)
712-712: Consider adding clarifying comment for delay conversion.The
10multiplier converts GIF centiseconds to milliseconds, and50sets a reasonable minimum. A brief comment would help future maintainers.Apply this diff:
const avg=gF.reduce((s,f)=>s+f.delay,0)/gF.length; -aT=setInterval(step,Math.max(avg*10,50)); +// GIF delays are in centiseconds; convert to ms with 50ms minimum +aT=setInterval(step,Math.max(avg*10,50));
956-975: Consider using a more sophisticated color quantization algorithm.The current bit-reduction approach works but may produce poor results for images with many colors. A median-cut or octree quantization would preserve colors better.
The current implementation is functional for a simple tool, but if image quality is important, consider implementing median-cut quantization or using a library. The current approach progressively drops LSBs which can cause significant banding in gradients.
Example libraries:
- RgbQuant.js
- quantize.js
However, this would add dependency size, so only consider if image quality issues are reported.
878-878: Use more precise regex for hex color parsing.The pattern
/\w\w/gmatches any two word characters, but/[0-9a-fA-F]{2}/gmore precisely matches hex digits. While color inputs are browser-controlled, the more specific pattern is technically more correct.Apply this diff:
let t=+gId('bt').value, dt=c.getImageData(0,0,c.canvas.width,c.canvas.height), - b=gId('bg').value.match(/\w\w/g).map(x=>parseInt(x,16)); + b=gId('bg').value.match(/[0-9a-fA-F]{2}/g).map(x=>parseInt(x,16));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/data/matrixtool/matrixtool.htm(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
wled00/data/**/*.{htm,html,css,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use tabs for indentation in web files (.htm/.html/.css/.js) under wled00/data
Files:
wled00/data/matrixtool/matrixtool.htm
wled00/data/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
wled00/data/**: When modifying web UI files, runnpm run buildto regenerate embedded headers before any firmware build
For web UI changes, edit files only under wled00/data (not firmware or generated files)
Files:
wled00/data/matrixtool/matrixtool.htm
🧠 Learnings (2)
📚 Learning: 2025-10-05T15:41:15.790Z
Learnt from: DedeHai
PR: wled/WLED#4982
File: wled00/data/matrixtool/omggif.js:32-35
Timestamp: 2025-10-05T15:41:15.790Z
Learning: The file `wled00/data/matrixtool/omggif.js` is an external third-party library from https://github.com/deanm/omggif by Dean McNamee and should not be reviewed for code issues as it's maintained externally.
Applied to files:
wled00/data/matrixtool/matrixtool.htm
📚 Learning: 2025-10-05T15:24:05.535Z
Learnt from: CR
PR: wled/WLED#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.535Z
Learning: Applies to wled00/data/**/*.{htm,html,css,js} : Use tabs for indentation in web files (.htm/.html/.css/.js) under wled00/data
Applied to files:
wled00/data/matrixtool/matrixtool.htm
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
🔇 Additional comments (5)
wled00/data/matrixtool/matrixtool.htm (5)
469-477: Segment filtering correctly prevents NaN dimensions.The check
if(h>1&&w>1)properly filters out 1D segments and prevents NaN values from being stored in the dataset, as the dataset assignments are inside the conditional block.
697-713: GIF animation logic is well-structured.The function properly handles edge cases (no frames, single frame), clears existing intervals before starting new ones, and uses a sensible minimum frame delay to prevent excessive CPU usage.
640-668: GIF disposal handling correctly implements the GIF89a spec.The frame-by-frame decoding properly handles disposal modes:
- Mode 2: Restore to background (line 660)
- Mode 3: Restore to previous frame (line 661)
This ensures animated GIFs display correctly even with complex disposal requirements.
984-1005: Image playback correctly handles segment conflicts.The function detects when another segment is showing an image and prompts the user before switching. The conditional payload construction (lines 993-995) ensures the old segment is properly cleared (fx=0) when switching.
1072-1082: Token insertion properly handles edge cases.The function correctly preserves cursor position, respects the 64-character limit, and handles missing selection positions with nullish coalescing. The Math.min ensures the cursor doesn't overflow the string length.
| const w=+gId('w').value,h=+gId('h').value,fn=gId('fn').value.trim()||'image'; | ||
| const filename=`${fn}.gif`; |
There was a problem hiding this comment.
Sanitize user-provided filename to prevent path traversal.
The filename is constructed from user input without sanitization. While the server should validate, client-side sanitization provides defense-in-depth against path traversal attempts.
Apply this diff to sanitize the filename:
const w=+gId('w').value,h=+gId('h').value,fn=gId('fn').value.trim()||'image';
-const filename=`${fn}.gif`;
+// Sanitize filename: remove path separators and limit to safe characters
+const safeFn = fn.replace(/[^a-zA-Z0-9_-]/g, '_').substring(0, 26);
+const filename=`${safeFn}.gif`;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const w=+gId('w').value,h=+gId('h').value,fn=gId('fn').value.trim()||'image'; | |
| const filename=`${fn}.gif`; | |
| const w = +gId('w').value, | |
| h = +gId('h').value, | |
| fn = gId('fn').value.trim() || 'image'; | |
| // Sanitize filename: remove path separators and limit to safe characters | |
| const safeFn = fn.replace(/[^a-zA-Z0-9_-]/g, '_').substring(0, 26); | |
| const filename = `${safeFn}.gif`; |
🤖 Prompt for AI Agents
In wled00/data/matrixtool/matrixtool.htm around lines 899-900, the code builds
filename from user input without sanitization, allowing path traversal or
malformed names; sanitize the user-provided fn by removing any path separators
and traversal tokens (e.g., strip slashes and backslashes, remove any "../" or
"..\" sequences), allow only a safe subset of characters (e.g., letters, digits,
hyphen, underscore), collapse multiple dots so only a single basename remains,
fall back to 'image' if the sanitized name is empty, then append the fixed
'.gif' extension; ensure the resulting filename contains no directory components
and contains only the allowed characters before assigning to filename.
if @import is used, css minifying is skipped. changing to link instead and using writeChunks() saves 300 bytes
| <meta name="viewport" content="width=device-width, initial-scale=1.0"> | ||
| <meta name="author" content="@dedehai" /> | ||
| <link rel="shortcut icon" href="favicon.ico"> | ||
| <link rel="stylesheet" href="style.css"> <!-- do not use @import url() for css file, it prevents minifying! --> |
There was a problem hiding this comment.
FYI style.css is already minified and included in .h file, served by webserver. It will now be included twice.
There was a problem hiding this comment.
nope, I am excluding it.
There was a problem hiding this comment.
the update in this commit is to properly minify which I found does not happen if not using "link"
DedeHai
changed the title
|
@DedeHai i found an old feature request, which asks for integrating the "lametric icons" PixelArt database:
|
- better gif palette calculation - use global palette for gif animation export instead of per frame to generate much smaller files - code consolidation
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
wled00/wled_server.cpp (2)
12-17: Consider aligning feature flag patterns for consistency.The PxMagic feature uses an opt-in pattern (
#ifdef WLED_ENABLE_PXMAGIC), while the MatrixTool feature uses an opt-out pattern (#ifndef WLED_DISABLE_MATRIXTOOL). This inconsistency creates confusion about default behavior and makes the codebase harder to maintain.Consider standardizing to one pattern. Options:
- Option 1 (Recommended): Use opt-out for both (consistent with other WLED features):
-#ifdef WLED_ENABLE_PXMAGIC +#ifndef WLED_DISABLE_PXMAGIC #include "html_pxmagic.h" #endif
- Option 2: Use opt-in for both:
-#ifndef WLED_DISABLE_MATRIXTOOL +#ifdef WLED_ENABLE_MATRIXTOOL #include "html_matrixtool.h" #endifNote: This same change would need to be applied to lines 606-618 for the route handlers.
606-618: Same feature flag pattern inconsistency as in the includes section.The route handlers mirror the inconsistent pattern from lines 12-17, where PxMagic uses opt-in (
#ifdef WLED_ENABLE_PXMAGIC) and MatrixTool uses opt-out (#ifndef WLED_DISABLE_MATRIXTOOL).If you align the patterns at lines 12-17 as suggested, apply the same change here:
Option 1 (if using opt-out for both):
- #ifdef WLED_ENABLE_PXMAGIC + #ifndef WLED_DISABLE_PXMAGIC static const char _pxmagic_htm[] PROGMEM = "/pxmagic.htm"; server.on(_pxmagic_htm, HTTP_GET, [](AsyncWebServerRequest *request) { handleStaticContent(request, FPSTR(_pxmagic_htm), 200, FPSTR(CONTENT_TYPE_HTML), PAGE_pxmagic, PAGE_pxmagic_length); }); #endifOption 2 (if using opt-in for both):
- #ifndef WLED_DISABLE_MATRIXTOOL + #ifdef WLED_ENABLE_MATRIXTOOL static const char _matrixtool_htm[] PROGMEM = "/matrixtool.htm"; server.on(_matrixtool_htm, HTTP_GET, [](AsyncWebServerRequest *request) { handleStaticContent(request, FPSTR(_matrixtool_htm), 200, FPSTR(CONTENT_TYPE_HTML), PAGE_matrixtool, PAGE_matrixtool_length); }); #endif
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tools/cdata.js(3 hunks)wled00/data/index.js(0 hunks)wled00/wled_server.cpp(2 hunks)
💤 Files with no reviewable changes (1)
- wled00/data/index.js
🧰 Additional context used
📓 Path-based instructions (2)
tools/cdata.js
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use tools/cdata.js to convert web assets to embedded C++ headers; do not reimplement this logic elsewhere
Files:
tools/cdata.js
wled00/**/*.cpp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for C++ source files (.cpp)
Files:
wled00/wled_server.cpp
🧠 Learnings (19)
📓 Common learnings
Learnt from: DedeHai
Repo: wled/WLED PR: 4982
File: wled00/data/matrixtool/omggif.js:32-35
Timestamp: 2025-10-05T15:41:15.810Z
Learning: The file `wled00/data/matrixtool/omggif.js` is an external third-party library from https://github.com/deanm/omggif by Dean McNamee and should not be reviewed for code issues as it's maintained externally.
Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with defined constants when meaningful constants exist in the codebase. For example, suggest replacing hardcoded "32" with WLED_MAX_SEGNAME_LEN if the context relates to segment name length limits.
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to tools/cdata.js : Use tools/cdata.js to convert web assets to embedded C++ headers; do not reimplement this logic elsewhere
Applied to files:
tools/cdata.js
📚 Learning: 2025-10-05T15:41:15.810Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4982
File: wled00/data/matrixtool/omggif.js:32-35
Timestamp: 2025-10-05T15:41:15.810Z
Learning: The file `wled00/data/matrixtool/omggif.js` is an external third-party library from https://github.com/deanm/omggif by Dean McNamee and should not be reviewed for code issues as it's maintained externally.
Applied to files:
tools/cdata.jswled00/wled_server.cpp
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/data/** : When modifying web UI files, run `npm run build` to regenerate embedded headers before any firmware build
Applied to files:
tools/cdata.js
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/data/** : For web UI changes, edit files only under wled00/data (not firmware or generated files)
Applied to files:
tools/cdata.jswled00/wled_server.cpp
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/html_*.h : DO NOT edit generated embedded web header files (wled00/html_*.h)
Applied to files:
tools/cdata.jswled00/wled_server.cpp
📚 Learning: 2025-09-14T18:43:59.338Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4932
File: wled00/data/cpal/cpal.htm:14-14
Timestamp: 2025-09-14T18:43:59.338Z
Learning: In WLED, static web pages like cpal.htm are served directly at the root URL level (e.g., "/cpal.htm") via handleStaticContent() from embedded page data, regardless of their physical source folder location in the repository. Relative script references like "common.js" resolve correctly from the served URL path, not the source file path.
Applied to files:
tools/cdata.jswled00/wled_server.cpp
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/html_*.h : Always commit generated html_*.h files along with related source changes
Applied to files:
tools/cdata.jswled00/wled_server.cpp
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/data/index.htm : Main web interface entry file is index.htm; ensure it remains present and functional
Applied to files:
tools/cdata.js
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/data/**/*.{htm,html,css,js} : Use tabs for indentation in web files (.htm/.html/.css/.js) under wled00/data
Applied to files:
tools/cdata.js
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/data/settings*.htm : Name settings pages as settings*.htm within the web UI
Applied to files:
tools/cdata.js
📚 Learning: 2025-11-14T13:37:11.994Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.
Applied to files:
wled00/wled_server.cpp
📚 Learning: 2025-08-26T11:51:21.817Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Applied to files:
wled00/wled_server.cpp
📚 Learning: 2025-11-14T13:37:30.955Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with defined constants when meaningful constants exist in the codebase. For example, suggest replacing hardcoded "32" with WLED_MAX_SEGNAME_LEN if the context relates to segment name length limits.
Applied to files:
wled00/wled_server.cpp
📚 Learning: 2025-08-28T08:09:20.630Z
Learnt from: mval-sg
Repo: wled/WLED PR: 4876
File: wled00/xml.cpp:0-0
Timestamp: 2025-08-28T08:09:20.630Z
Learning: The WLED codebase has opportunities for refactoring hardcoded array bounds (like the "15" used for DMX channels) to use sizeof(array)/sizeof(array[0]) for more maintainable code, but such changes should be done consistently across the entire codebase in a dedicated refactoring effort.
Applied to files:
wled00/wled_server.cpp
📚 Learning: 2025-10-10T18:34:06.550Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4997
File: wled00/e131.cpp:33-44
Timestamp: 2025-10-10T18:34:06.550Z
Learning: In WLED's DDP packet handling (ws.cpp and e131.cpp), only prevent out-of-bounds memory access rather than enforcing DDP spec compliance. Don't check the 1440-byte spec limit—accept out-of-spec packets assuming correct encoding. The bounds check `maxDataIndex = c + numLeds * ddpChannelsPerLed; if (maxDataIndex > dataLen) reject` is sufficient and already covers the timecode flag case (when c=4) without needing separate validation.
Applied to files:
wled00/wled_server.cpp
📚 Learning: 2025-08-31T03:38:14.114Z
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 4891
File: wled00/FX.cpp:3333-3349
Timestamp: 2025-08-31T03:38:14.114Z
Learning: WLED PacMan effect (wled00/FX.cpp): Keep pacmancharacters_t position fields as signed int (not int16_t). Maintainer preference (blazoncek) prioritizes avoiding potential overhead/regressions over minor RAM savings. Avoid type shrinking here unless memory pressure is demonstrated.
Applied to files:
wled00/wled_server.cpp
📚 Learning: 2025-08-29T00:26:15.808Z
Learnt from: ksedgwic
Repo: wled/WLED PR: 4883
File: usermods/usermod_v2_skystrip/rest_json_client.h:6-14
Timestamp: 2025-08-29T00:26:15.808Z
Learning: WLED uses a vendored ArduinoJson library (version 6) located at "src/dependencies/json/ArduinoJson-v6.h" which is included through wled.h. Usermods should not directly include ArduinoJson headers but instead rely on wled.h for ArduinoJson symbols. The standard pattern is to include wled.h and use JsonObject, JsonArray, DynamicJsonDocument, etc. without additional includes.
Applied to files:
wled00/wled_server.cpp
📚 Learning: 2025-08-29T00:26:15.808Z
Learnt from: ksedgwic
Repo: wled/WLED PR: 4883
File: usermods/usermod_v2_skystrip/rest_json_client.h:6-14
Timestamp: 2025-08-29T00:26:15.808Z
Learning: In WLED projects, ArduinoJson.h is not directly included via #include <ArduinoJson.h> - the ArduinoJson symbols are made available through the WLED build system and wled.h transitive includes, so explicitly adding #include <ArduinoJson.h> is not necessary and may not work.
Applied to files:
wled00/wled_server.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32dev_debug)
🔇 Additional comments (3)
tools/cdata.js (3)
29-29: LGTM! Output array updated for matrixtool.The addition of
"wled00/html_matrixtool.h"is consistent with other output file declarations and properly corresponds to the generation call at line 249.
249-249: LGTM! Matrix Tool generation configured correctly.The invocation properly generates the matrixtool HTML header with
inlineCss = false, allowing external CSS references. This is consistent with the PR's approach to sharestyle.cssacross multiple pages while keeping the matrixtool bundle size manageable.
129-135: Verified: CSS serving infrastructure properly supports external references.The
/style.cssroute is correctly configured in the server (wled_server.cpp:354-356) to serve the embedded CSS resource. The matrixtool.htm's relativehref="style.css"will properly resolve to this root-level route. WheninlineCss=falseis passed to the new parameter:
strict: falseallows the external reference without throwing errorsstylesheets: falsepreserves the external CSS linkThe default
inlineCss=truemaintains backward compatibility for pages that inline CSS. No further changes needed.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
wled00/data/matrixtool/matrixtool.htm (5)
434-434: Validate host parameter to prevent potential SSRF.The
hostparameter from the URL is used directly for API calls without validation, as previously flagged. This remains a security concern.
582-596: Add integrity verification for external downloads.The hardcoded GitHub URL lacks integrity checks, as previously flagged. If the domain is compromised or a MITM attack occurs, malicious content could be installed on the device. This remains a security concern.
979-980: Sanitize user-provided filename to prevent path traversal.The filename is constructed from user input without sanitization, as previously flagged. While the server should validate, client-side sanitization provides defense-in-depth against path traversal attempts.
1151-1156: Add error handling for text update.The
txtUpfunction fires off the update without waiting for or checking the response, as previously flagged. This could silently fail. The apply button handler (lines 1159-1170) properly handles responses with await and error checking.
467-474: Guard against NaN in width calculation.While height has a fallback (
h=(stopY-startY)||1), width doesn't. Ifstoporstartis undefined,wbecomes NaN and propagates todataset.w, breaking downstream calculations. Consider either adding a fallback for width or filtering out segments where dimensions are invalid (width < 2 or height < 2), as suggested in previous discussion.Apply this diff to add validation:
j.seg.forEach(({id,n,start,stop,startY,stopY,fx})=>{ - const w=stop-start,h=(stopY-startY)||1; + const w=(stop-start)||1,h=(stopY-startY)||1; + // Skip segments that are too small for matrix operations + if(w < 2 || h < 2) return; const t = (n || `Segment ${id}`) + (h>1 ? ` (${w}x${h})` : ` (${w}px)`) + (fx===53 ? ' [Image]' : (fx===122 ? ' [Scrolling Text]' : ''));
🧹 Nitpick comments (1)
wled00/data/matrixtool/matrixtool.htm (1)
315-315: Replace magic numbers with defined constants.Several hardcoded limits appear throughout:
26(line 315, 689): filename length limit64(line 329, 1117, 1152, 1160): text content length limit320(line 961, 1004): WLED GIF dimension limit256(line 1004 comment): 2D GIF maximum dimensionIf these correspond to WLED-defined constants (e.g., filename limits, segment name limits, image dimension limits), replace the literals with named constants to improve maintainability and prevent inconsistencies.
Based on learnings.
Also applies to: 329-329, 689-689, 961-961, 1004-1004, 1117-1117
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/data/matrixtool/matrixtool.htm(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
wled00/data/**/*.{htm,html,css,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use tabs for indentation in web files (.htm/.html/.css/.js) under wled00/data
Files:
wled00/data/matrixtool/matrixtool.htm
wled00/data/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
wled00/data/**: When modifying web UI files, runnpm run buildto regenerate embedded headers before any firmware build
For web UI changes, edit files only under wled00/data (not firmware or generated files)
Files:
wled00/data/matrixtool/matrixtool.htm
🧠 Learnings (14)
📓 Common learnings
Learnt from: DedeHai
Repo: wled/WLED PR: 4982
File: wled00/data/matrixtool/omggif.js:32-35
Timestamp: 2025-10-05T15:41:15.810Z
Learning: The file `wled00/data/matrixtool/omggif.js` is an external third-party library from https://github.com/deanm/omggif by Dean McNamee and should not be reviewed for code issues as it's maintained externally.
Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with defined constants when meaningful constants exist in the codebase. For example, suggest replacing hardcoded "32" with WLED_MAX_SEGNAME_LEN if the context relates to segment name length limits.
📚 Learning: 2025-10-05T15:41:15.810Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4982
File: wled00/data/matrixtool/omggif.js:32-35
Timestamp: 2025-10-05T15:41:15.810Z
Learning: The file `wled00/data/matrixtool/omggif.js` is an external third-party library from https://github.com/deanm/omggif by Dean McNamee and should not be reviewed for code issues as it's maintained externally.
Applied to files:
wled00/data/matrixtool/matrixtool.htm
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/data/**/*.{htm,html,css,js} : Use tabs for indentation in web files (.htm/.html/.css/.js) under wled00/data
Applied to files:
wled00/data/matrixtool/matrixtool.htm
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/**/!(html_*)*.h : Use 2-space indentation for non-generated C++ header files (.h)
Applied to files:
wled00/data/matrixtool/matrixtool.htm
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/**/*.cpp : Use 2-space indentation for C++ source files (.cpp)
Applied to files:
wled00/data/matrixtool/matrixtool.htm
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/data/** : For web UI changes, edit files only under wled00/data (not firmware or generated files)
Applied to files:
wled00/data/matrixtool/matrixtool.htm
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/data/index.htm : Main web interface entry file is index.htm; ensure it remains present and functional
Applied to files:
wled00/data/matrixtool/matrixtool.htm
📚 Learning: 2025-11-14T05:48:44.673Z
Learnt from: DedeHai
Repo: wled/WLED PR: 5040
File: wled00/image_loader.cpp:84-96
Timestamp: 2025-11-14T05:48:44.673Z
Learning: In WLED (wled00/FX_2Dfcn.cpp), the Segment::setPixelColorXY() function performs internal bounds checking against vWidth() and vHeight(), returning early if coordinates are out of bounds. No additional guards are needed when calling this function, even in upscaling loops where coordinates might exceed segment dimensions.
Applied to files:
wled00/data/matrixtool/matrixtool.htm
📚 Learning: 2025-04-24T09:31:06.879Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4658
File: wled00/led.cpp:90-91
Timestamp: 2025-04-24T09:31:06.879Z
Learning: In the WLED codebase, the `getSegment()` function guards against out-of-bounds segment IDs by falling back to the main segment ID, and `getFirstSelectedSegId()` falls back to `getMainSegmentId()` if no segments are selected, ensuring no crashes when used through the `setValuesFromFirstSelectedSeg()` macro.
Applied to files:
wled00/data/matrixtool/matrixtool.htm
📚 Learning: 2025-04-24T09:31:06.879Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4658
File: wled00/led.cpp:90-91
Timestamp: 2025-04-24T09:31:06.879Z
Learning: In the WLED codebase, the `getSegment()` function guards against out-of-bounds segment IDs, and `getFirstSelectedSegId()` falls back to `getMainSegmentId()` if no segments are selected, ensuring no crashes when used through the `setValuesFromFirstSelectedSeg()` macro.
Applied to files:
wled00/data/matrixtool/matrixtool.htm
📚 Learning: 2025-08-26T11:51:21.817Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Applied to files:
wled00/data/matrixtool/matrixtool.htm
📚 Learning: 2025-11-14T13:37:30.955Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, verify that file operations (especially file.open()) respect LittleFS filename limitations. Assume default WLED configuration with LittleFS default filename limit of 255 bytes. Do not assume extreme configuration values like WLED_MAX_SEGNAME_LEN = 512 which would not be standard configurations.
Applied to files:
wled00/data/matrixtool/matrixtool.htm
📚 Learning: 2025-11-14T13:37:11.994Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, file operations (especially file.open()) should be checked to ensure they respect LittleFS filename limitations. The default LittleFS filename limit is 255 bytes (LFS_NAME_MAX). Reviews should assume default WLED configuration defines and not extreme edge-case values (e.g., WLED_MAX_SEGNAME_LEN = 512 would not be standard). File paths should be validated to stay within the 255-byte limit.
Applied to files:
wled00/data/matrixtool/matrixtool.htm
📚 Learning: 2025-09-12T17:29:43.826Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4923
File: wled00/FX.cpp:4883-4901
Timestamp: 2025-09-12T17:29:43.826Z
Learning: In WLED’s web UI, only one slider value (e.g., SEGMENT.intensity or SEGMENT.custom1) changes at a time; code relying on this may use simplified change guards, though presets/JSON can still update multiple fields atomically.
Applied to files:
wled00/data/matrixtool/matrixtool.htm
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32c3dev_qio)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (esp32dev_debug)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
wled00/data/matrixtool/matrixtool.htm (2)
432-434: Validate host parameter to prevent SSRF.The
hostparameter from the URL query string is used directly to construct API endpoints without validation. This enables Server-Side Request Forgery (SSRF) attacks where an attacker could redirect requests to internal services or arbitrary external hosts.Apply this diff to add validation:
(async()=>{ const params=new URLSearchParams(window.location.search); - wu=`http://${params.get('host')||window.location.host}`; + const host = params.get('host') || window.location.host; + // Validate host matches expected pattern (hostname or hostname:port) + if (!/^[a-zA-Z0-9.-]+(:\d+)?$/.test(host)) { + console.error('Invalid host parameter'); + wu = `http://${window.location.host}`; + } else { + wu = `http://${host}`; + }
977-982: Sanitize filename to prevent path traversal.The filename is constructed from user input without sanitization, which could allow path traversal attempts (e.g.,
../,..\\). While the server should validate filenames, client-side sanitization provides defense-in-depth.Apply this diff to sanitize the filename:
gId('up').onclick = async () => { if (!gF || !gI) return; // no image const w = +gId('w').value, h = +gId('h').value, fn = gId('fn').value.trim() || 'image'; - const filename = `${fn}.gif`; + // Sanitize filename: remove path separators and limit to safe characters + const safeFn = fn.replace(/[^a-zA-Z0-9_-]/g, '_').substring(0, 26) || 'image'; + const filename = `${safeFn}.gif`; const repl = iL.includes(filename);
🧹 Nitpick comments (3)
wled00/data/matrixtool/matrixtool.htm (3)
582-596: Consider adding integrity verification for external downloads.The tool downloads files from GitHub without verifying integrity (hash/signature). While the risk is moderate, adding a subresource integrity check would provide defense-in-depth against compromised sources or MITM attacks.
If you prefer to add integrity checks, compute the SHA-256 hash of the expected file and verify before upload:
const f = await fetch(url); if (!f.ok) throw new Error("Download failed " + f.status); const blob = await f.blob(); // Verify integrity const buffer = await blob.arrayBuffer(); const hash = await crypto.subtle.digest('SHA-256', buffer); const hashHex = Array.from(new Uint8Array(hash)) .map(b => b.toString(16).padStart(2, '0')).join(''); const expectedHash = 'YOUR_EXPECTED_SHA256_HERE'; // Update with actual hash if (hashHex !== expectedHash) { throw new Error("Integrity check failed"); } const fd = new FormData(); fd.append("data", blob, fileGz);Alternatively, serving these tools directly from the firmware or documenting the expected hash would also mitigate the risk.
1157-1162: Add error handling for text updates.The
txtUpfunction fires the POST request without checking the response or handling errors. Silent failures could confuse users who see no feedback when updates fail. Consider adding basic error handling similar to the apply button handler.Apply this diff to add error handling:
function txtUp(){ const id=+gId('segT').value,txt=gId('txt').value.trim().slice(0,64); const j={on:true,seg:{id,fx:122,n:txt,sx:+gId('sx').value,ix:+gId('ix').value,c1:+gId('c1').value,c2:+gId('c2').value,c3:+gId('c3').value,o1:gId('o1').checked,o3:gId('o3').checked}}; - fetch(`${wu}/json/state`,{method:'POST',body:JSON.stringify(j)}); - segLoad(); + fetch(`${wu}/json/state`,{method:'POST',body:JSON.stringify(j)}) + .then(r => r.json()) + .then(out => { + if(out.success !== false) segLoad(); + }) + .catch(err => console.error('Text update failed:', err)); }
10-10: TODO comments indicate planned improvements.Two TODO comments highlight areas for future enhancement:
- Line 10: Sequential loading and code size optimization for common.js integration
- Line 535: Handling 503 errors when switching tabs causes unloaded images
These are noted for tracking but don't block the current implementation.
If you'd like help addressing either TODO, I can suggest approaches for:
- Implementing sequential script loading to optimize initial page load
- Adding retry logic or debouncing for image loading to handle 503 errors
Based on learnings
Also applies to: 535-535
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/data/matrixtool/matrixtool.htm(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
wled00/data/**/*.{htm,html,css,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use tabs for indentation in web files (.htm/.html/.css/.js) under wled00/data
Files:
wled00/data/matrixtool/matrixtool.htm
wled00/data/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
wled00/data/**: When modifying web UI files, runnpm run buildto regenerate embedded headers before any firmware build
For web UI changes, edit files only under wled00/data (not firmware or generated files)
Files:
wled00/data/matrixtool/matrixtool.htm
🧠 Learnings (22)
📓 Common learnings
Learnt from: DedeHai
Repo: wled/WLED PR: 4982
File: wled00/data/matrixtool/omggif.js:32-35
Timestamp: 2025-10-05T15:41:15.810Z
Learning: The file `wled00/data/matrixtool/omggif.js` is an external third-party library from https://github.com/deanm/omggif by Dean McNamee and should not be reviewed for code issues as it's maintained externally.
Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with defined constants when meaningful constants exist in the codebase. For example, suggest replacing hardcoded "32" with WLED_MAX_SEGNAME_LEN if the context relates to segment name length limits.
📚 Learning: 2025-10-05T15:41:15.810Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4982
File: wled00/data/matrixtool/omggif.js:32-35
Timestamp: 2025-10-05T15:41:15.810Z
Learning: The file `wled00/data/matrixtool/omggif.js` is an external third-party library from https://github.com/deanm/omggif by Dean McNamee and should not be reviewed for code issues as it's maintained externally.
Applied to files:
wled00/data/matrixtool/matrixtool.htm
📚 Learning: 2025-09-12T17:29:43.826Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4923
File: wled00/FX.cpp:4883-4901
Timestamp: 2025-09-12T17:29:43.826Z
Learning: In WLED’s web UI, only one slider value (e.g., SEGMENT.intensity or SEGMENT.custom1) changes at a time; code relying on this may use simplified change guards, though presets/JSON can still update multiple fields atomically.
Applied to files:
wled00/data/matrixtool/matrixtool.htm
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/data/**/*.{htm,html,css,js} : Use tabs for indentation in web files (.htm/.html/.css/.js) under wled00/data
Applied to files:
wled00/data/matrixtool/matrixtool.htm
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/**/!(html_*)*.h : Use 2-space indentation for non-generated C++ header files (.h)
Applied to files:
wled00/data/matrixtool/matrixtool.htm
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/**/*.cpp : Use 2-space indentation for C++ source files (.cpp)
Applied to files:
wled00/data/matrixtool/matrixtool.htm
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/data/** : For web UI changes, edit files only under wled00/data (not firmware or generated files)
Applied to files:
wled00/data/matrixtool/matrixtool.htm
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/data/index.htm : Main web interface entry file is index.htm; ensure it remains present and functional
Applied to files:
wled00/data/matrixtool/matrixtool.htm
📚 Learning: 2025-11-14T05:48:44.673Z
Learnt from: DedeHai
Repo: wled/WLED PR: 5040
File: wled00/image_loader.cpp:84-96
Timestamp: 2025-11-14T05:48:44.673Z
Learning: In WLED (wled00/FX_2Dfcn.cpp), the Segment::setPixelColorXY() function performs internal bounds checking against vWidth() and vHeight(), returning early if coordinates are out of bounds. No additional guards are needed when calling this function, even in upscaling loops where coordinates might exceed segment dimensions.
Applied to files:
wled00/data/matrixtool/matrixtool.htm
📚 Learning: 2025-04-24T09:31:06.879Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4658
File: wled00/led.cpp:90-91
Timestamp: 2025-04-24T09:31:06.879Z
Learning: In the WLED codebase, the `getSegment()` function guards against out-of-bounds segment IDs by falling back to the main segment ID, and `getFirstSelectedSegId()` falls back to `getMainSegmentId()` if no segments are selected, ensuring no crashes when used through the `setValuesFromFirstSelectedSeg()` macro.
Applied to files:
wled00/data/matrixtool/matrixtool.htm
📚 Learning: 2025-04-24T09:31:06.879Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4658
File: wled00/led.cpp:90-91
Timestamp: 2025-04-24T09:31:06.879Z
Learning: In the WLED codebase, the `getSegment()` function guards against out-of-bounds segment IDs, and `getFirstSelectedSegId()` falls back to `getMainSegmentId()` if no segments are selected, ensuring no crashes when used through the `setValuesFromFirstSelectedSeg()` macro.
Applied to files:
wled00/data/matrixtool/matrixtool.htm
📚 Learning: 2025-08-26T11:51:21.817Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Applied to files:
wled00/data/matrixtool/matrixtool.htm
📚 Learning: 2025-11-14T13:37:30.955Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, verify that file operations (especially file.open()) respect LittleFS filename limitations. Assume default WLED configuration with LittleFS default filename limit of 255 bytes. Do not assume extreme configuration values like WLED_MAX_SEGNAME_LEN = 512 which would not be standard configurations.
Applied to files:
wled00/data/matrixtool/matrixtool.htm
📚 Learning: 2025-11-14T13:37:11.994Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, file operations (especially file.open()) should be checked to ensure they respect LittleFS filename limitations. The default LittleFS filename limit is 255 bytes (LFS_NAME_MAX). Reviews should assume default WLED configuration defines and not extreme edge-case values (e.g., WLED_MAX_SEGNAME_LEN = 512 would not be standard). File paths should be validated to stay within the 255-byte limit.
Applied to files:
wled00/data/matrixtool/matrixtool.htm
📚 Learning: 2025-10-20T09:41:50.374Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4995
File: wled00/FX.cpp:5209-5212
Timestamp: 2025-10-20T09:41:50.374Z
Learning: In WLED 2D effects, SEGMENT.length() returns the physical pixel count (upper bound), while SEG_W * SEG_H can be smaller depending on mapping (mirroring/grouping). For per-cell state buffers (e.g., mode_2Dgameoflife in wled00/FX.cpp), allocate using SEGMENT.length() to ensure sufficient capacity and avoid reallocations, while bounding indexing by cols*rows.
Applied to files:
wled00/data/matrixtool/matrixtool.htm
📚 Learning: 2025-10-20T09:38:51.997Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4995
File: wled00/FX.cpp:5223-5226
Timestamp: 2025-10-20T09:38:51.997Z
Learning: WLED matrices: each dimension (SEG_W, SEG_H) is limited to ≤255; 256 or larger per side is not supported/feasible on ESP32, so effects should assume per-dimension max 255.
Applied to files:
wled00/data/matrixtool/matrixtool.htm
📚 Learning: 2025-11-14T13:37:11.994Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.
Applied to files:
wled00/data/matrixtool/matrixtool.htm
📚 Learning: 2025-04-30T05:41:03.633Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4667
File: usermods/user_fx/user_fx.cpp:27-30
Timestamp: 2025-04-30T05:41:03.633Z
Learning: In WLED, the Segment::allocateData() method already includes optimization to check if data is allocated and sufficiently sized, handling buffer reuse to reduce memory fragmentation. Adding an external check like `if (SEGENV.data == nullptr && !SEGENV.allocateData(dataSize))` is unnecessary and could be problematic, as it bypasses proper size verification.
Applied to files:
wled00/data/matrixtool/matrixtool.htm
📚 Learning: 2025-10-10T18:34:06.550Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4997
File: wled00/e131.cpp:33-44
Timestamp: 2025-10-10T18:34:06.550Z
Learning: In WLED's DDP packet handling (ws.cpp and e131.cpp), only prevent out-of-bounds memory access rather than enforcing DDP spec compliance. Don't check the 1440-byte spec limit—accept out-of-spec packets assuming correct encoding. The bounds check `maxDataIndex = c + numLeds * ddpChannelsPerLed; if (maxDataIndex > dataLen) reject` is sufficient and already covers the timecode flag case (when c=4) without needing separate validation.
Applied to files:
wled00/data/matrixtool/matrixtool.htm
📚 Learning: 2025-06-07T15:58:42.579Z
Learnt from: willmmiles
Repo: wled/WLED PR: 4712
File: wled00/FX_fcn.cpp:992-993
Timestamp: 2025-06-07T15:58:42.579Z
Learning: In WLED segments, the backing pixel buffer is always allocated to the full physical segment size, but effect rendering only uses a subset when mirror or grouping are enabled. For 2D segments, this subset is vWidth() * vHeight(), and for 1D segments it's vLength(). Color processing operations like fade_out should iterate over this rendering subset, not the full allocated buffer.
Applied to files:
wled00/data/matrixtool/matrixtool.htm
📚 Learning: 2025-11-14T13:37:30.955Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with defined constants when meaningful constants exist in the codebase. For example, suggest replacing hardcoded "32" with WLED_MAX_SEGNAME_LEN if the context relates to segment name length limits.
Applied to files:
wled00/data/matrixtool/matrixtool.htm
📚 Learning: 2025-08-28T08:09:20.630Z
Learnt from: mval-sg
Repo: wled/WLED PR: 4876
File: wled00/xml.cpp:0-0
Timestamp: 2025-08-28T08:09:20.630Z
Learning: The WLED codebase has opportunities for refactoring hardcoded array bounds (like the "15" used for DMX channels) to use sizeof(array)/sizeof(array[0]) for more maintainable code, but such changes should be done consistently across the entire codebase in a dedicated refactoring effort.
Applied to files:
wled00/data/matrixtool/matrixtool.htm
|
I added support for 1D strips now that we updated the gif player to properly handle that. Also updated to work again with latest changes to /edit handling. @netmindz this is good to merge if no one has any concerns. I'd like to get more people to test and give feedback to find potential issues on different browsers/devices. |
4 participants
replaces the pixel magic tool.
This implements a much more feature-rich and user friendly way of displaying images.
Features:
Image conversion tool:
Scrolling Text tool:
Total additional flash usage: 2.5k
For consistency I also updated the button styling in style.css: it now also uses a hover action.
Took some inspiration from @Manut38 (https://github.com/Manut38/WLED-GifPlayer-UI)
Summary by CodeRabbit
New Features
UI Changes
Performance/Behavior
Style